Station settings photo verification UI component #203
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive photo verification functionality to the WeatherXM iOS application. The changes span multiple layers of the application, including presentation, domain, and data layers. New views, view models, repositories, and use cases have been added to support a complete photo verification workflow. The implementation includes features like photo upload, camera permission handling, localization support, and integration with device information screens. Changes
Sequence DiagramsequenceDiagram
participant User
participant PhotoIntroView
participant GalleryView
participant PhotoVerificationStateView
participant DeviceInfoViewModel
User->>PhotoIntroView: Begin photo verification
PhotoIntroView->>GalleryView: Navigate to gallery
User->>GalleryView: Upload photos
GalleryView->>PhotoVerificationStateView: Submit photos
PhotoVerificationStateView->>DeviceInfoViewModel: Update device photos
DeviceInfoViewModel-->>User: Show verification status
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 16
🔭 Outside diff range comments (3)
PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoFactory.swift (1)
Line range hint
137-178: Inconsistent button behavior for photo verification.When there are existing photos, the button returns nil, preventing users from updating or re-verifying photos. It might be useful to provide a button for adding or replacing photos. Also, consider adding test coverage to confirm that the UI transitions are correct for various states (e.g., from no photos to some photos).
wxm-ios.xcodeproj/project.pbxproj (2)
iOS deployment targets are inconsistent across the project
The deployment target verification reveals inconsistencies across different targets:
- Main app and widget extension: iOS 16.0
- DataLayer and DomainLayer: Mix of iOS 14.0 and 16.0
- PresentationLayer: Mix of iOS 14.0 and 15.0
- Toolkit: Mix of iOS 16.0 and 16.2
This inconsistency could lead to runtime issues. The deployment target should be aligned across all modules to ensure proper functionality, especially since the widget extension requires iOS 16.0.
🔗 Analysis chain
Line range hint
3783-3821: Verify iOS deployment target configurationThe deployment target is set to iOS 16.0 for the widget extension. This matches the main app target which also requires iOS 16.0. This ensures consistency across all targets.
Also applies to: 3858-3896
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify deployment target consistency across targets grep -r "IPHONEOS_DEPLOYMENT_TARGET" .Length of output: 3761
Missing RewardAnalytics group folder despite related files
The verification reveals a discrepancy in the project structure:
- RewardAnalytics files exist (
RewardAnalyticsChartFactory.swift,RewardAnalyticsView.swift,RewardAnalyticsViewModel.swift) but are not organized under a dedicated group folder like other features- Other feature folders (PhotoVerification, RewardBoosts, RewardAnnotations) are properly organized as mentioned
Consider moving RewardAnalytics-related files into a dedicated group folder to maintain consistent project organization.
🔗 Analysis chain
Line range hint
1242-1246: Verify new feature organization in project structureThe new photo verification feature files are properly organized under dedicated group folders:
- PhotoVerification
- RewardAnalytics
- RewardBoosts
- RewardAnnotations
The initial results show that while PhotoVerification, RewardBoosts, and RewardAnnotations folders exist in the expected location, RewardAnalytics wasn't found. Let's verify if it exists elsewhere or if there's a discrepancy in the project configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify group folder structure exists find . -type d -name "PhotoVerification" -o -name "RewardAnalytics" -o -name "RewardBoosts" -o -name "RewardAnnotations"Length of output: 286
Script:
#!/bin/bash # Check if RewardAnalytics folder exists in other locations fd RewardAnalytics # Check project.pbxproj for RewardAnalytics references rg "RewardAnalytics" "*.pbxproj"Length of output: 381
🧹 Nitpick comments (51)
wxm-ios/DataLayer/DataLayer/Networking/Mock/Jsons/get_device_info_helium.json (2)
22-33: LGTM! Consider adding schema validation.The reward split structure is well-formed with appropriate fields for wallet addresses, stakes, and rewards. However, to ensure data integrity in tests, consider adding JSON schema validation to enforce:
- Stake percentages sum to 100%
- Reward amounts are non-negative
- Wallet addresses follow the correct format
24-24: Review test wallet addresses.The wallet addresses differ only in their last digit (645e vs 643e). If these are test addresses, consider making them more distinct to better represent real-world scenarios and prevent potential confusion during testing.
- "wallet": "0xc4E253863371fdeD8e414731DB951F4C17Bc645e", + "wallet": "0x1234567890123456789012345678901234567890", - "wallet": "0xc4E253863371fdeD8e414731DB951F4C17Bc643e", + "wallet": "0x9876543210987654321098765432109876543210",Also applies to: 29-29
PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoFactory.swift (2)
Line range hint
47-95: Good addition of photo verification state in description.The revamped description logic properly covers various states (uploading, content, empty, loading) for photo verification. Make sure to implement corresponding snapshot tests for each state to avoid regressions.
131-132: Nil URL for photos is expected, but consider placeholders.Returning nil for images under the “.photos” case is acceptable if the design requires no image. As a user experience enhancement, consider using a placeholder image, especially if UI elements expect an image or if the user might interpret the absence of an image as a loading error.
PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoViewModel+Content.swift (1)
41-42: Fix trailing whitespaceThere's trailing whitespace after the comma in line 42.
- [.name, .frequency, .reboot], + [.name, .frequency, .reboot],🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 42-42: Lines should not have trailing whitespace
(trailing_whitespace)
wxm-ios/Resources/Localizable/LocalizableConstants.swift (1)
17-17: Consider adding documentation for the new localization keys.To help other developers understand the context and proper usage of these new localization keys, consider adding documentation comments explaining their intended use in the photo verification UI.
Example documentation:
+ /// Used for photo verification UI navigation case new + /// Used for photo verification UI back button case back + /// Used for photo verification UI exit button case exit + /// Used for photo verification UI skip button case skipAlso applies to: 23-25
PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoRowView.swift (2)
24-33: Consider accessibility improvements for the badgeWhile the badge implementation is solid, consider these accessibility enhancements:
- Add accessibility label to clarify the badge's meaning for VoiceOver users
- Consider making the badge color configurable for different states/contexts
if let badge = row.badge { Text(badge.uppercased()) .foregroundColor(Color(colorEnum: .wxmPrimary)) .font(.system(size: CGFloat(.littleCaption), weight: .bold)) .padding(.horizontal, CGFloat(.smallSidePadding)) .padding(.vertical, CGFloat(.minimumPadding)) .background { Capsule().fill(Color(colorEnum: .layer2)) } + .accessibilityLabel("\(badge) badge") }
Line range hint
169-174: Consider adding more preview casesThe current preview only demonstrates a row with a badge. Consider adding a Group with multiple previews to showcase different states:
- Row without badge
- Row with different badge texts
- Row with different color schemes (light/dark mode)
struct DeviceInfoRowView_Previews: PreviewProvider { static var previews: some View { + Group { + // Current case - with badge DeviceInfoRowView(row: DeviceInfoRowView.Row(title: "TItle", badge: "New", description: "This is a **desription**".attributedMarkdown!, imageUrl: URL(string: "https://i0.wp.com/weatherxm.com/wp-content/uploads/2023/12/Home-header-image-1200-x-1200-px-5.png?w=1200&ssl=1"), buttonInfo: .init(icon: nil, title: "Button title"), warning: .desructive("This action is not reversible!")) {}) + + // Without badge + DeviceInfoRowView(row: DeviceInfoRowView.Row(title: "TItle", + badge: nil, + description: "This is a **desription**".attributedMarkdown!, + imageUrl: nil, + buttonInfo: nil, + warning: nil) {}) + } } }wxm-ios/Toolkit/Toolkit/Utils/FileStorage+.swift (2)
24-26: Fix code style issuesThe catch clause has incorrect brace placement and spacing.
Apply these style fixes:
- catch let error { - print("Unable to create directory \(error)") - } + } catch let error { + print("Unable to create directory \(error)") + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Opening braces should be preceded by a single space and on the same line as the declaration
(opening_brace)
1-30: Consider architectural improvementsAs this is part of a photo verification feature, consider these architectural improvements:
- Add documentation comments for public APIs
- Consider moving file operations to background queue
- Implement proper logging strategy for file system operations
- Add unit tests for directory operations
Would you like me to provide examples for any of these improvements?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Opening braces should be preceded by a single space and on the same line as the declaration
(opening_brace)
[Warning] 23-23: Else and catch should be on the same line, one space after the previous declaration
(statement_position)
wxm-ios/DataLayer/DataLayer/Networking/Mock/Jsons/get_device_info_m5.json (1)
26-33: Document reward calculation formula and precision handlingThe reward amounts have high precision (14 decimal places). Consider:
- Documenting the reward calculation formula
- Specifying the required precision for reward amounts
- Adding comments about potential rounding considerations
PresentationLayer/UIComponents/Modifiers/WXMShareModifier.swift (2)
43-45: Add documentation and validation to helper methodThe helper method would benefit from documentation and input validation.
Consider this improvement:
+ /// Creates a ShareFileItemSource for the given file URL + /// - Parameter file: The URL of the file to share + /// - Returns: A ShareFileItemSource instance if the file is valid, nil otherwise func getItemSource(file: URL) -> ShareFileItemSource? { + guard file.isFileURL else { return nil } ShareFileItemSource(fileUrl: file) }
64-64: Remove extra blank lineThere's an extra blank line that should be removed to maintain consistent formatting.
- deinit {🧰 Tools
🪛 GitHub Check: swiftLint
[warning] 64-64:
Vertical Whitespace Violation: Limit vertical whitespace to a single empty line; currently 2 (vertical_whitespace)🪛 SwiftLint (0.57.0)
[Warning] 64-64: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
PresentationLayer/UIComponents/Screens/DeviceInfo/PhotoVerificationState/PhotoVerificationStateViewModel.swift (1)
21-29: Potential improvement for fetch timing.
The 2-seconds delay is presumably for demonstration or simulated load. If used in production, consider removing or making it configurable.wxm-ios/DataLayer/DataLayer/RepositoryImplementations/PhotosRepositoryImpl.swift (2)
45-59: Backup/remote deletion fallback.
The code gracefully handles local vs. remote images. Consider using more robust error handling or user feedback if remote deletion fails.
102-112: Omit unused enum argument.
You can remove “_” in the failure case since it’s not used.- case .failure(_): + case .failure:🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 110-110: Arguments can be omitted when matching enums with associated values if they are not used
(empty_enum_arguments)
PresentationLayer/UIComponents/BaseComponents/FailSuccess/SuccessView.swift (1)
14-15: Avoid redundant optional initialization.
Per static analysis, initializing an optional with nil is redundant.- var infoCustomView: AnyView? = nil + var infoCustomView: AnyView?🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 15-15: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🪛 GitHub Check: swiftLint
[warning] 15-15:
Redundant Optional Initialization Violation: Initializing an optional variable with nil is redundant (redundant_optional_initialization)PresentationLayer/UIComponents/Screens/PhotoVerification/GalleryView/GalleryViewModel.swift (4)
61-73: Robust error handling on deletion.
The current deletion flow gracefully handles errors by showing a toast. However, you could consider logging these errors or providing retry actions for a better user experience, particularly if network or file IO issues may be transient.
79-81: Clarify placeholder logic for “handleUploadButtonTap” method.
This method only sets showShareSheet without genuinely uploading the images. Consider adding a proper upload flow or clarifying the comment to avoid confusion.
98-98: Line length warning.
SwiftLint flagged line 98 as exceeding the recommended 180-character limit. Reducing line length enhances readability.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 98-98: Line should be 180 characters or less; currently it has 198 characters
(line_length)
111-114: Empty hash implementation.
The conformance to HashableViewModel is left empty. If the view model needs to be part of a hashed collection or used in comparison, consider implementing a meaningful hash. Otherwise, it may be safe to remove.PresentationLayer/UIComponents/Screens/PhotoVerification/GalleryView/GalleryView.swift (2)
57-75: Improve loading/error handling for image states.
The LazyImage displays a ProgressView for loading but does not handle potential load failures (e.g., invalid URLs). Consider handling error states (like a fallback image) in the onFailure closure.
172-186: Smooth plus-button transitions.
Currently you use a fade transition (.opacity). If you want a more dynamic user experience, consider combining a scale or slide transition to show or hide the plus button more smoothly.PresentationLayer/UIComponents/Screens/ClaimDevice/ClaimDeviceContainer/ClaimDeviceContainerViewModel.swift (3)
169-170: Semantic clarity for skip vs. continue.
Using “skipPhotoVerificationForNow” alongside “continueToPhotoVerification” is clear but could benefit from more consistent wording (e.g., both as “Skip Photo Verification” / “Go to Photo Verification”).
220-220: Trailing whitespace detected.
Removing trailing whitespace helps maintain a clean code style.🧰 Tools
🪛 GitHub Check: swiftLint
[warning] 220-220:
Trailing Whitespace Violation: Lines should not have trailing whitespace (trailing_whitespace)🪛 SwiftLint (0.57.0)
[Warning] 220-220: Lines should not have trailing whitespace
(trailing_whitespace)
251-262: Alert logic and analytics.
“showSkipPhotoAlert” is a great approach, but consider also logging the user’s choice (i.e., skipping verification) for analytics consistency.PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoViewModel.swift (2)
88-90: Consider user feedback on empty photos.
Currently, nonexistent or empty photo arrays are simply set to nil. You might provide a user-facing indicator or fallback UI if device photos are unavailable.
454-474: Modularizing customViewFor(field:).
As more Field cases expand, consider splitting field-specific logic (e.g. photos, rewardSplit) into separate helper methods. This makes the code more organized and easier to maintain.PresentationLayer/UIComponents/Modifiers/ButtonStyles/WXMButtonOpacityStyle.swift (1)
10-18: Add documentation for the button style.Consider adding documentation comments explaining the style's purpose and usage patterns.
+/// A button style that provides visual feedback through opacity changes. +/// - Pressed state: 70% opacity +/// - Disabled state: 40% opacity +/// - Normal state: 100% opacity struct WXMButtonOpacityStyle: ButtonStyle {Consider making opacity values configurable.
The hardcoded opacity values could be made configurable for better reusability across different UI contexts.
struct WXMButtonOpacityStyle: ButtonStyle { + let pressedOpacity: Double + let disabledOpacity: Double + + init(pressedOpacity: Double = 0.7, disabledOpacity: Double = 0.4) { + self.pressedOpacity = pressedOpacity + self.disabledOpacity = disabledOpacity + } @Environment(\.isEnabled) private var isEnabled: Bool func makeBody(configuration: Configuration) -> some View { configuration.label - .opacity(configuration.isPressed ? 0.7 : 1.0) - .opacity(isEnabled ? 1.0 : 0.4) + .opacity(configuration.isPressed ? pressedOpacity : 1.0) + .opacity(isEnabled ? 1.0 : disabledOpacity) } }wxm-ios/DomainLayer/DomainLayer/DomainRepositoryInterfaces/PhotosRepository.swift (2)
14-14: Remove trailing whitespace.There is trailing whitespace after the method declaration.
- func setTermsAccepted(_ termsAccepted: Bool) + func setTermsAccepted(_ termsAccepted: Bool)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
22-26: Enhance error descriptions.Consider providing more descriptive raw values for the error cases to improve error messages.
public enum PhotosError: String, Error { - case imageNotFound - case failedToSaveImage - case failedToDeleteImage + case imageNotFound = "The requested image could not be found in the repository" + case failedToSaveImage = "Failed to save the image to the repository" + case failedToDeleteImage = "Failed to delete the image from the repository" }PresentationLayer/UIComponents/Modifiers/ButtonStyles/WXMCapsuleButtonStyle.swift (1)
10-24: LGTM! Consider adding documentation.The button style implementation is clean and follows SwiftUI best practices with proper state handling and visual feedback.
Consider adding documentation comments to describe:
- The purpose and usage of this button style
- The visual states (enabled/disabled/pressed)
- Example usage in a view
+/// A capsule-shaped button style with primary/disabled states and press animation +/// +/// Example usage: +/// ``` +/// Button("Action") { } +/// .buttonStyle(WXMCapsuleButtonStyle()) +/// ``` struct WXMCapsuleButtonStyle: ButtonStyle {wxm-ios/DomainLayer/DomainLayer/UseCases/PhotoGalleryUseCase.swift (1)
12-45: Document error handling and add error types.The use case should clearly document possible errors and define custom error types.
Add error types at the top of the file:
public enum PhotoGalleryError: Error { case invalidImage case saveFailed case deleteFailed case permissionDenied }Add documentation for methods that can throw:
+ /// Saves the image with optional metadata + /// - Parameters: + /// - image: The image to save + /// - metadata: Optional metadata dictionary + /// - Throws: PhotoGalleryError + /// - Returns: URL string of saved image public func saveImage(_ image: UIImage, metadata: NSDictionary?) async throws -> String? {PresentationLayer/UIComponents/BaseComponents/Indication/Indication.swift (2)
26-30: Fix inconsistent indentation.The indentation in the method calls is inconsistent and makes the code harder to read.
- .WXMCardStyle(backgroundColor: bgColor, - insideHorizontalPadding: 0.0, - insideVerticalPadding: 0.0, - cornerRadius: cornerRadius) - .strokeBorder(color: borderColor, lineWidth: borderWidth, radius: cornerRadius) + .WXMCardStyle( + backgroundColor: bgColor, + insideHorizontalPadding: 0.0, + insideVerticalPadding: 0.0, + cornerRadius: cornerRadius + ) + .strokeBorder( + color: borderColor, + lineWidth: borderWidth, + radius: cornerRadius + )
Line range hint
54-61: Enhance preview with multiple states.The preview should demonstrate different states of the indication modifier.
#Preview { - WeatherStationCard(device: .mockDevice, followState: nil) - .indication(show: .constant(true), borderColor: .red, bgColor: .red.opacity(0.5)) { - Text(verbatim: "Indication") - .padding() - } + VStack(spacing: 20) { + // Normal state + WeatherStationCard(device: .mockDevice, followState: nil) + .indication( + show: .constant(true), + borderColor: .red, + bgColor: .red.opacity(0.5) + ) { + Text(verbatim: "Error Indication") + .padding() + } + + // Success state + WeatherStationCard(device: .mockDevice, followState: nil) + .indication( + show: .constant(true), + borderColor: .green, + bgColor: .green.opacity(0.5) + ) { + Text(verbatim: "Success Indication") + .padding() + } + } + .padding() }PresentationLayer/Constants/Dimensions.swift (2)
29-29: Consider adding documentation for the new corner radius.While the implementation is correct, it would be helpful to add a comment explaining the intended usage context for this specific corner radius size to help other developers choose the appropriate dimension.
Add documentation above the enum case:
+ /// 8pt corner radius, suitable for smaller UI components that need subtle rounding case smallCornerRadiusAlso applies to: 78-79
Line range hint
18-18: Consider adding unit tests for the new photo verification flow.The implementation follows good architectural practices with proper separation of concerns. However, to ensure reliability:
- Add unit tests for the
DeviceInfoUseCase.getDevicePhotosmethod- Add integration tests for the repository implementation
- Consider adding UI tests using the mock scheme mentioned in the PR description
Would you like help creating a test plan or generating the test implementations?
Also applies to: 42-45
PresentationLayer/UIComponents/BaseComponents/FailSuccess/FailSuccessStateObject.swift (1)
16-17: Remove redundant nil initializationThe explicit nil initialization for
infoCustomViewis redundant as optional properties are automatically initialized to nil.-var infoCustomView: AnyView? = nil +var infoCustomView: AnyView?🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 17-17: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🪛 GitHub Check: swiftLint
[warning] 17-17:
Redundant Optional Initialization Violation: Initializing an optional variable with nil is redundant (redundant_optional_initialization)wxm-ios/DomainLayer/DomainLayer/Extensions/UserDefaults+Constants.swift (1)
49-50: Fix line length to comply with SwiftLintThe line exceeds the 180 character limit. Consider breaking it into multiple lines for better readability.
- let keys: [GenericKey] = [.hideWalletTimestamp, .sortByDevicesOption, .filterDevicesOption, .groupByDevicesOption, .userDevicesFollowStates, .userDevices, .lastSurveyId, .lastInfoBannerId, .arePhotoVerificationTermsAccepted] + let keys: [GenericKey] = [ + .hideWalletTimestamp, + .sortByDevicesOption, + .filterDevicesOption, + .groupByDevicesOption, + .userDevicesFollowStates, + .userDevices, + .lastSurveyId, + .lastInfoBannerId, + .arePhotoVerificationTermsAccepted + ]🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 49-49: Line should be 180 characters or less; currently it has 227 characters
(line_length)
PresentationLayer/UIComponents/Screens/DeviceInfo/PhotoVerificationState/PhotoVerificationStateView.swift (1)
33-91: Consider breaking down the photosView function into smaller components.The function currently spans 52 lines. Consider extracting the photo grid and warning card into separate view components for better maintainability.
Example refactor:
- func photosView(photos: [URL], isFailed: Bool) -> some View { + func photosView(photos: [URL], isFailed: Bool) -> some View { + VStack(spacing: CGFloat(.smallSpacing)) { + PhotoGridView(photos: photos, lastPhotoOverlay: viewModel.morePhotosCount) + if isFailed { + UploadErrorView() + } + } + } + + private func PhotoGridView(photos: [URL], lastPhotoOverlay: Int) -> some View { // Extract grid implementation here + } + + private func UploadErrorView() -> some View { // Extract warning card implementation here + }🧰 Tools
🪛 GitHub Check: swiftLint
[warning] 34-34:
Function Body Length Violation: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines (function_body_length)🪛 SwiftLint (0.57.0)
[Warning] 34-34: Function body should span 50 lines or less excluding comments and whitespace: currently spans 52 lines
(function_body_length)
PresentationLayer/UIComponents/Screens/PhotoVerification/Intro/PhotoIntroExamplesView.swift (2)
10-38: Add documentation for the isDestructive flag.While the implementation is clean, consider adding documentation to explain the purpose and impact of the
isDestructiveflag, as it affects multiple aspects of the view's appearance.struct PhotoIntroExamplesView: View { + /// When true, the view displays error states with red icons and reduced scaling. + /// When false, the view shows success states with green icons and normal scaling. var isDestructive: Bool = false
74-82: Consider making Example struct public for reusability.The
Examplestruct could be useful in other parts of the app. Consider making it public and documenting its purpose.- struct Example: Identifiable { + /// Represents a photo example with its associated bullet points + public struct Example: Identifiable {PresentationLayer/Constants/AssetEnum.swift (1)
123-134: Consider grouping related asset cases with comments.The new photo verification assets would be more maintainable with clear grouping comments.
+ // MARK: - Photo Verification Installation Examples case recommendedInstallation0 = "recommended_installation_0" case recommendedInstallation1 = "recommended_installation_1" case recommendedInstallation2 = "recommended_installation_2" case recommendedInstallation3 = "recommended_installation_3" case wrongInstallation0 = "wrong_installation_0" case wrongInstallation1 = "wrong_installation_1" case wrongInstallation2 = "wrong_installation_2" + + // MARK: - Photo Verification Icons case iconRotate = "icon_rotate" case iconSurface = "icon_surface" case iconNoFaces = "icon_no_faces" case iconMaxPhotos = "icon_max_photos" case photoVerificationCover = "photo_verification_cover"PresentationLayer/UIComponents/Screens/PhotoVerification/Intro/PhotoIntroView.swift (2)
14-68: Consider extracting the overlay into a separate view componentThe main view structure is well-organized, but the overlay implementation (lines 40-61) could be extracted into a separate view component to improve readability and reusability.
+ struct CloseButtonOverlay: View { + let dismissAction: () -> Void + var body: some View { + VStack { + HStack { + Button(action: dismissAction) { + Text(viewModel.closeButtonIcon.rawValue) + .font(.fontAwesome(font: .FAPro, size: CGFloat(.mediumFontSize))) + .foregroundColor(Color(colorEnum: .textDarkStable)) + .padding(CGFloat(.smallSidePadding)) + .background { + Circle().foregroundStyle(Color.black.opacity(0.4)) + } + } + Spacer() + } + Spacer() + } + .padding(.top, CGFloat(.smallSidePadding)) + .padding(.leading, CGFloat(.largeSidePadding)) + } + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 25-25: Limit vertical whitespace to a single empty line; currently 2
(vertical_whitespace)
216-245: Add accessibility identifiers for UI testingThe terms view contains important interactive elements (toggle and button) but lacks accessibility identifiers which are crucial for UI testing.
Toggle("", isOn: $viewModel.areTermsAccepted) .labelsHidden() .toggleStyle(WXMToggleStyle.Default) + .accessibilityIdentifier("termsToggle") Button { viewModel.handleBeginButtonTap() } label: { Text(LocalizableString.PhotoVerification.letsTakeTheFirstPhoto.localized) } .buttonStyle(WXMButtonStyle(textColor: .top, fillColor: .wxmPrimary)) .disabled(!viewModel.areTermsAccepted) +.accessibilityIdentifier("beginPhotoVerificationButton")PresentationLayer/UIComponents/Screens/MainScreen/MainScreenViewModel.swift (2)
297-303: Enhance error handling for photo purgingThe current implementation only prints errors to the console. Consider:
- Adding proper error logging
- Implementing a retry mechanism for failed purges
- Adding analytics tracking for failures
private func purgeSavedPhotos() { do { try photosUseCase.purgeImages() } catch { - print("Error purging photos: \(error)") + WXMLogger.shared.error("Failed to purge photos: \(error)") + WXMAnalytics.shared.track(.photosPurgeFailed) + // Consider implementing retry logic here } }
75-75: Consider deferring photo purge operationPurging photos during initialization could impact app startup time. Consider:
- Deferring this operation to a background task
- Using async/await to prevent blocking the main thread
PresentationLayer/UIComponents/ViewModelsFactory.swift (1)
264-282: LGTM: Photo verification view model factoriesThe new factory methods are well-structured and follow the existing patterns. Consider adding documentation comments to describe the purpose and usage of each new view model type.
+/// Creates a view model for the photo verification introduction screen static func getPhotoIntroViewModel() -> PhotoIntroViewModel { let useCase = SwinjectHelper.shared.getContainerForSwinject().resolve(PhotoGalleryUseCase.self)! return PhotoIntroViewModel(photoGalleryUseCase: useCase) } +/// Creates a view model for displaying photo verification instructions static func getPhotoInstructionsViewModel() -> PhotoInstructionsViewModel { let useCase = SwinjectHelper.shared.getContainerForSwinject().resolve(PhotoGalleryUseCase.self)! return PhotoInstructionsViewModel(photoGalleryUseCase: useCase) } +/// Creates a view model for managing the photo gallery display +/// - Parameter images: Array of image URLs to display in the gallery static func getGalleryViewModel(images: [String]) -> GalleryViewModel { let useCase = SwinjectHelper.shared.getContainerForSwinject().resolve(PhotoGalleryUseCase.self)! return GalleryViewModel(images: images, photoGalleryUseCase: useCase) } +/// Creates a view model for managing the photo verification state +/// - Parameter deviceId: The ID of the device being verified static func getPhotoVerificationStateViewModel(deviceId: String) -> PhotoVerificationStateViewModel { let useCase = SwinjectHelper.shared.getContainerForSwinject().resolve(DeviceInfoUseCase.self)! return PhotoVerificationStateViewModel(deviceId: deviceId, deviceInfoUseCase: useCase) }wxm-ios/Resources/Localizable/LocalizableString+PhotoVerification.swift (1)
67-78: Consider caching formatted strings for better performanceThe
localizedproperty formats strings on every access. For frequently accessed strings with parameters, consider caching the formatted result.+private var cachedString: String? var localized: String { + if let cached = cachedString { + return cached + } var localized = NSLocalizedString(key, comment: "") switch self { case .morePhotosToUpload(let count): localized = String(format: localized, count) + cachedString = localized default: break } return localized }PresentationLayer/UIComponents/Screens/ClaimDevice/ClaimDeviceContainer/ClaimHeliumContainerViewModel.swift (2)
124-128: Enhance simulator mock data for better testingThe current mock
BTWXMDeviceInfouses empty strings which might not effectively simulate real-world scenarios. Consider using realistic mock data to ensure proper testing in the simulator environment.#if targetEnvironment(simulator) - let result: Result<BTWXMDeviceInfo?, BluetoothHeliumError> = .success(.init(devEUI: "", claimingKey: "")) + let result: Result<BTWXMDeviceInfo?, BluetoothHeliumError> = .success(.init(devEUI: "MOCK-DEV-EUI-001", claimingKey: "MOCK-CLAIM-KEY-001")) #else
157-160: Remove unnecessary empty line for consistencyWhile the simulator bypass implementation is correct, there's an extra empty line that affects readability and differs from the style used in
setHeliumFrequency.#if targetEnvironment(simulator) return nil #endif - guard let btDevice else {
🛑 Comments failed to post (16)
wxm-ios/Toolkit/Toolkit/Utils/FileStorage+.swift (2)
10-16: 🛠️ Refactor suggestion
Improve robustness and clarity of
getDocumentsDirectory()The current implementation has potential issues:
- Using
FileManager.defaultinside an instance method is redundant- Direct array access could crash if no paths are returned
- Method could be static as it doesn't use instance properties
Consider this safer implementation:
public extension FileManager { - func getDocumentsDirectory() -> URL { - let paths = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask) - let documentsDirectory = paths[0] - return documentsDirectory + static func getDocumentsDirectory() -> URL? { + let paths = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask) + return paths.first } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public extension FileManager { static func getDocumentsDirectory() -> URL? { let paths = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask) return paths.first } }
18-29: 🛠️ Refactor suggestion
Enhance error handling in
createDirectory()The current implementation only prints errors to console, which isn't suitable for production code. Consider throwing errors to allow proper error handling by callers.
Consider this more robust implementation:
public extension URL { - func createDirectory() { + func createDirectory() throws { if !FileManager.default.fileExists(atPath: self.path) { - do { - try FileManager.default.createDirectory(at: self, withIntermediateDirectories: true, attributes: nil) - } - catch let error { - print("Unable to create directory \(error)") - } + try FileManager.default.createDirectory(at: self, withIntermediateDirectories: true, attributes: nil) } } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public extension URL { func createDirectory() throws { if !FileManager.default.fileExists(atPath: self.path) { try FileManager.default.createDirectory(at: self, withIntermediateDirectories: true, attributes: nil) } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Opening braces should be preceded by a single space and on the same line as the declaration
(opening_brace)
[Warning] 23-23: Else and catch should be on the same line, one space after the previous declaration
(statement_position)
wxm-ios/DomainLayer/DomainLayer.xcodeproj/project.pbxproj (1)
711-711: 💡 Codebase verification
⚠️ Potential issueInconsistent iOS deployment targets across workspace modules
The deployment target changes are inconsistent across the workspace:
- DataLayer: Mixed (14.0 and 16.0)
- DomainLayer: Mixed (14.0 and 16.0)
- PresentationLayer: Mixed (14.0 and 15.0)
- Toolkit: Mixed (16.0 and 16.2)
- Main app: Mixed (14.0, 16.0, and 17.0)
This inconsistency could lead to runtime issues. The modules need to be aligned to maintain compatibility:
- Either keep iOS 14.0 support across all modules
- Or coordinate the iOS 16.0+ requirement across all configurations and modules
🔗 Analysis chain
Breaking Change: iOS deployment target increased to 16.0
The deployment target has been increased from iOS 14.0 to iOS 16.0 across all configurations. This is a breaking change that:
- Drops support for iOS 14-15
- May affect existing users who haven't upgraded
- Should be documented in release notes
Please ensure this change is intentional and properly communicated to users.
Also applies to: 873-873, 909-909
Let's check the deployment targets in all these project files to verify if this is a coordinated change across the workspace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other targets in the workspace maintain compatibility with older iOS versions # Search for deployment target definitions in other xcodeproj files fd -e pbxproj -x grep -l "IPHONEOS_DEPLOYMENT_TARGET"Length of output: 329
Script:
#!/bin/bash # Check deployment target values in all project files for file in $(fd -e pbxproj); do echo "=== $file ===" grep -A 1 "IPHONEOS_DEPLOYMENT_TARGET =" "$file" doneLength of output: 4508
wxm-ios/DataLayer/DataLayer.xcodeproj/project.pbxproj (1)
723-723:
⚠️ Potential issueBreaking Change: iOS deployment target increased to 16.0
The minimum iOS version requirement has been increased from 15.0 to 16.0 across all build configurations. This is a breaking change that will prevent users on iOS 15 from using the app.
Please verify if this change is intentional and necessary for the photo verification feature. If it's required for using specific iOS 16 photo-related APIs, document this requirement. Otherwise, consider maintaining iOS 15 support.
Also applies to: 883-883, 919-919
PresentationLayer/UIComponents/Modifiers/WXMShareModifier.swift (2)
33-41: 🛠️ Refactor suggestion
Add validation and error handling for file URLs
The current implementation assumes all files are valid and accessible. Consider adding validation to ensure better error handling.
Consider this improvement:
func presentShare(sourceView: UIView?) { var items: [Any] = [text] - items.append(contentsOf: files.map { getItemSource(file: $0) }) + items.append(contentsOf: files.compactMap { file in + guard FileManager.default.fileExists(atPath: file.path) else { + Logger.warning("File not found at path: \(file.path)") + return nil + } + return getItemSource(file: file) + }) let activityController = WXMActivityViewController(activityItems: items, applicationActivities: nil)Committable suggestion skipped: line range outside the PR's diff.
81-105: 🛠️ Refactor suggestion
Optimize image handling and add error handling
The current implementation loads the entire image into memory and lacks proper error handling.
Consider these improvements:
- Use image data provider for better memory management
- Add error handling for image loading
- Add logging for debugging purposes
class ShareFileItemSource: NSObject, UIActivityItemSource { let fileUrl: URL + private var imageProvider: NSItemProvider? init(fileUrl: URL) { self.fileUrl = fileUrl + super.init() + setupImageProvider() } + private func setupImageProvider() { + guard let image = UIImage(contentsOfFile: fileUrl.path()) else { + Logger.error("Failed to load image at path: \(fileUrl.path())") + return + } + imageProvider = NSItemProvider(object: image) + } func activityViewControllerPlaceholderItem(_ activityViewController: UIActivityViewController) -> Any { UIImage() } func activityViewController(_ activityViewController: UIActivityViewController, itemForActivityType activityType: UIActivity.ActivityType?) -> Any? { + Logger.debug("Sharing file: \(fileUrl.lastPathComponent)") fileUrl } func activityViewControllerLinkMetadata(_ activityViewController: UIActivityViewController) -> LPLinkMetadata? { - guard let image = UIImage(contentsOfFile: fileUrl.path()) else { + guard let imageProvider = self.imageProvider else { return nil } - let imageProvider = NSItemProvider(object: image) let metadata = LPLinkMetadata() metadata.imageProvider = imageProvider + metadata.title = fileUrl.lastPathComponent return metadata } }Committable suggestion skipped: line range outside the PR's diff.
wxm-ios/DataLayer/DataLayer/Networking/Mock/Jsons/get_user_devices.json (2)
212-214:
⚠️ Potential issueUpdate mock data timestamps
The device is marked as
"isActive": truebut contains outdated timestamps from January 2023. This inconsistency might affect testing of active/inactive device states and photo verification features.Consider updating the timestamps to recent dates:
"isActive": true, - "lastWeatherStationActivity": "2023-01-25T10:20:08-06:00", - "lastActiveAt": "2023-01-25T10:20:08-06:00", + "lastWeatherStationActivity": "2024-01-25T10:20:08-06:00", + "lastActiveAt": "2024-01-25T10:20:08-06:00",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."isActive": true, "lastWeatherStationActivity": "2024-01-25T10:20:08-06:00", "lastActiveAt": "2024-01-25T10:20:08-06:00",
220-232: 💡 Codebase verification
⚠️ Potential issueBased on the search results, I can now generate the final response since I have enough context about how pressure values are handled in the app:
Unrealistic pressure value needs adjustment for proper UI rendering
The pressure value of 603.77 hPa in the mock data is problematically low and could affect UI testing since:
- The app properly handles pressure values through dedicated formatters (
WeatherFormatter.getPressureLiterals) and unit conversions- Pressure is displayed in multiple UI components including charts (
ChartCardTypes,ForecastChartTypes) and device details- The value is stored in CoreData (
DBWeather) and used for analyticsSuggested fix:
"current_weather": { - "pressure": 603.776966380233, + "pressure": 1013.7,The new value of 1013.7 hPa represents a typical sea-level pressure that will provide more realistic test conditions for:
- Unit conversion between hPa and inHg
- Chart rendering with proper Y-axis scaling
- Weather condition analysis
🔗 Analysis chain
Review unrealistic weather values in mock data
Several weather values in the mock data are unrealistic and might affect testing:
- Wind speed of 47.62 m/s (171 km/h) represents hurricane-force winds
- Pressure of 603.77 hPa is impossibly low (typical sea-level pressure is ~1013 hPa)
These extreme values might cause UI rendering issues or affect business logic testing.
Consider adjusting to more realistic values:
"current_weather": { "timestamp": "2023-01-25T10:20:08-06:00", "temperature": 20.338020711659738, "humidity": 59, - "wind_speed": 47.62365157837036, + "wind_speed": 4.62, "wind_gust": 25.762633951364656, "wind_direction": 130, "uv_index": 7, "precipitation": 1.0720520490349283, - "pressure": 603.776966380233, + "pressure": 1013.7, "feels_like": 20.338020711659738, "icon": "drizzle" }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these extreme values are handled in the UI layer rg -A 5 "pressure|wind_speed" --glob "**/*.swift"Length of output: 44684
PresentationLayer/UIComponents/Screens/PhotoVerification/GalleryView/GalleryView.swift (1)
88-114: 🛠️ Refactor suggestion
Consider labeling destructive actions.
The delete button uses a trash icon, but no confirmation is displayed before deletion. A confirmation step or a secondary prompt might reduce accidental deletions.PresentationLayer/UIComponents/Screens/DeviceInfo/DeviceInfoViewModel.swift (1)
196-196: 🛠️ Refactor suggestion
Fetch photos alongside device info.
This is a good addition. However, consider a unified approach to error handling. For example, if photo fetching fails, you might show a toast or tie it into the same flow that handles other device info errors for consistency.wxm-ios/DomainLayer/DomainLayer/DomainRepositoryInterfaces/PhotosRepository.swift (2)
18-18:
⚠️ Potential issueFix typo in method name.
The method name contains a typo: "reqeust" should be "request".
- func reqeustCameraPermission() async -> AVAuthorizationStatus + func requestCameraPermission() async -> AVAuthorizationStatus📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func requestCameraPermission() async -> AVAuthorizationStatus
12-20: 🛠️ Refactor suggestion
Add documentation for public protocol.
As this is a public interface, it should be well-documented with clear usage instructions and parameter descriptions.
+/// Protocol defining the interface for managing photos within the application. +/// This includes handling terms acceptance, image storage, and camera permissions. public protocol PhotosRepository: Sendable { + /// Indicates whether the user has accepted the terms for photo usage. var areTermsAccepted: Bool { get } + + /// Updates the acceptance status of photo terms. + /// - Parameter termsAccepted: The new acceptance status func setTermsAccepted(_ termsAccepted: Bool) + + /// Saves an image to the repository with optional metadata. + /// - Parameters: + /// - image: The UIImage to save + /// - metadata: Optional metadata associated with the image + /// - Returns: The URL string where the image was saved + /// - Throws: PhotosError if the save operation fails func saveImage(_ image: UIImage, metadata: NSDictionary?) async throws -> String? + /// Deletes an image from the repository. + /// - Parameter imageUrl: The URL of the image to delete + /// - Throws: PhotosError if the delete operation fails func deleteImage(_ imageUrl: String) throws + /// Returns the current camera authorization status. func getCameraPermission() -> AVAuthorizationStatus + /// Requests camera permission from the user. + /// - Returns: The resulting authorization status func requestCameraPermission() async -> AVAuthorizationStatus + /// Removes all images from the repository. + /// - Throws: PhotosError if the purge operation fails func purgeImages() throws }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Protocol defining the interface for managing photos within the application. /// This includes handling terms acceptance, image storage, and camera permissions. public protocol PhotosRepository: Sendable { /// Indicates whether the user has accepted the terms for photo usage. var areTermsAccepted: Bool { get } /// Updates the acceptance status of photo terms. /// - Parameter termsAccepted: The new acceptance status func setTermsAccepted(_ termsAccepted: Bool) /// Saves an image to the repository with optional metadata. /// - Parameters: /// - image: The UIImage to save /// - metadata: Optional metadata associated with the image /// - Returns: The URL string where the image was saved /// - Throws: PhotosError if the save operation fails func saveImage(_ image: UIImage, metadata: NSDictionary?) async throws -> String? /// Deletes an image from the repository. /// - Parameter imageUrl: The URL of the image to delete /// - Throws: PhotosError if the delete operation fails func deleteImage(_ imageUrl: String) throws /// Returns the current camera authorization status. func getCameraPermission() -> AVAuthorizationStatus /// Requests camera permission from the user. /// - Returns: The resulting authorization status func requestCameraPermission() async -> AVAuthorizationStatus /// Removes all images from the repository. /// - Throws: PhotosError if the purge operation fails func purgeImages() throws }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
wxm-ios/DomainLayer/DomainLayer/UseCases/PhotoGalleryUseCase.swift (2)
38-40:
⚠️ Potential issueFix typo in method name.
There's a typo in the repository method call:
reqeustCameraPermissionshould berequestCameraPermission.- await photosRepository.reqeustCameraPermission() + await photosRepository.requestCameraPermission()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public func requestCameraPermission() async -> AVAuthorizationStatus { await photosRepository.requestCameraPermission() }
26-28: 🛠️ Refactor suggestion
Add input validation and memory management for image saving.
The image saving operation should validate input parameters and consider memory management for large images.
public func saveImage(_ image: UIImage, metadata: NSDictionary?) async throws -> String? { + guard image.size.width > 0, image.size.height > 0 else { + throw PhotoGalleryError.invalidImage + } + + // Compress image if needed to prevent memory issues + let maxDimension: CGFloat = 2048 + let scaledImage = image.resized(to: maxDimension) + try await photosRepository.saveImage(image, metadata: metadata) }Committable suggestion skipped: line range outside the PR's diff.
PresentationLayer/UIComponents/Screens/DeviceInfo/PhotoVerificationState/PhotoVerificationStateView.swift (1)
78-85:
⚠️ Potential issueImplement the retry upload action.
The retry upload button has an empty action closure. This needs to be implemented to handle upload retry functionality.
Button { - + viewModel.handleRetryUpload() } label: {Committable suggestion skipped: line range outside the PR's diff.
wxm-ios/Resources/Localizable/LocalizableString+PhotoVerification.swift (1)
120-122:
⚠️ Potential issueFix typo in localization key
There's a typo in the localization key for possible obstacles.
-case .checkShowingPossibleObastacle: +case .checkShowingPossibleObstacle -"photo_verification_check_showing_possible_obastacle" +"photo_verification_check_showing_possible_obstacle"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.case .checkShowingPossibleObstacle: "photo_verification_check_showing_possible_obstacle" case .checkFromTheSide:
There was a problem hiding this comment.
I found some differences with the designs:
- When more than 2 photos are available, in the designs there is no "+1" or "+X" in the overlay of the second photo. It's just the overlay.
- When clicking the photos and going into the gallery, instead of the "X" icon it should be a "Back button" and the popup displayed is wrong as it's the one shown from the claiming process.
- There is a weird inner loading animation in the card, which is not in the designs currently. I suggest keeping the same loading animation for all, and show the screen when everything is ready (e.g. when both API calls are completed). Otherwise we need to show errors in a different way etc etc and we will get into more states.
I couldn't test the in progress and error uploading states.
|
@PavlosTze everything is fixed. |
pantelisss
deleted the
feature/fe-1358-station-settings-photo-verification-ui-component
branch
Why?
Photo verification in the settings screen
How?
PhotoVerificationStateViewis the view that shows the device's photos and the logic is in its view model (PhotoVerificationStateViewModel)DeviceinfoViewModelwhich observes the UI state (loading, content, uploading)Testing
Run the mock scheme and ensure everything looks as expected
Additional context
fixes fe-1358
Summary by CodeRabbit
Release Notes
New Features
AssetEnumfor various installation and icon states.smallCornerRadiusto theDimensionenum for UI customization.FontIconenum with new icons for enhanced UI options.DisplayedLinksenum with a new URL for terms and conditions.PhotoVerificationStateViewandGalleryViewfor photo management.PhotosRepositoryandPhotoGalleryUseCasefor photo handling functionalities.Bug Fixes
CardWarningViewandFailView.Chores